Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SQS Listener EventBridgeMessage annotation and converter #1307

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

SQS Listener EventBridgeMessage annotation and converter #1307

wants to merge 5 commits into from

Conversation

frjonsen
Copy link
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Core goal was to add a new annotation, @EventBridgeMessage, as well as the corresponding converter and argument resolver. I took the liberty of splitting out common functionality SnsMessageConverter to an abstract WrappedMessageConverter to reduce duplication.

💡 Motivation and Context

Further context in this issue: #1272

💚 How did you test it?

Added tests similar to those of the @SnsNotificationMessage. The test resource eventBridgeMessage is taken partially from https://docs.aws.amazon.com/AmazonS3/latest/userguide/ev-events.html, though the payload was written by hand to match the POJO.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

@github-actions github-actions bot added component: sqs SQS integration related issue type: documentation Documentation or Samples related issue labels Dec 19, 2024
@frjonsen
Copy link
Contributor Author

I was unsure what to fill in as version, in the places they were mentioned. I wrote 3.3.0 as that seemed to be the next unreleased version. Should this be changed to something else, or will it be updated once it is determined which version this would be included in?

@tomazfernandes
Copy link
Contributor

Hey @frjonsen, sorry for the delay.

We've just merged a PR that changes a few things in the SnsNotification part of the project, if you can please rebase your branch and see what needs to be changed.

For the overall design I have two suggestions.

  1. If I understand correctly, the Wrapped abstract class will define the conversion hint when it's a list. How about we instead of the abstract class, we use an internal SmartMessageConverter to delegate to when we figure out the list's type? This way we'd be able to use it with any other message converters when it's a list. Makes sense?

  2. This is optional, but how about we create a EventBridgeMessage<MyPojo> class that would contain both the deserialized POJO and the event bridge metadata? We could use it internally instead of manipulating the JSON, and users could have access to all metadata if they want. So we'd be able to use both @EventBridgeMessage MyPojo and @EventBridgeMessage EventBridgeMessage<MyPojo>, and their list variants.

Thanks and please let me know your thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maciejwalkowiak maciejwalkowiak Awaiting requested review from maciejwalkowiak maciejwalkowiak is a code owner

@MatejNedic MatejNedic Awaiting requested review from MatejNedic MatejNedic is a code owner

@tomazfernandes tomazfernandes Awaiting requested review from tomazfernandes tomazfernandes is a code owner

Assignees
No one assigned
Labels
component: sqs SQS integration related issue type: documentation Documentation or Samples related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@frjonsen @tomazfernandes